@hvitved @tamasvajk : What do you think?

I think this sounds overall like a (great) win, as DCA also confirms. The changes in results for cs/thread-unsafe-icryptotransform-field-in-class are merely the result of that query not filtering to fields from source code, like it should, so I have opened #18178 for that.

I vaguely remember that we started extracting private symbols because we needed them for stub generation. This is the PR where this was changed: #6000 and it also links to this #5664.

We needed these additional symbols when stubbing the .net framework, but I don't remember any longer for which API. The below might or might not be the case that caused the problem. I think when the below was extracted into a DB without private symbols, then we couldn't generate a stub that compiled.

public abstract class A
{
    public A(int i) => throw null;
}
public class B : A
{
    private B(int i) : base(i) => throw null; // not in the DB
    public B(int i, string s): this(i) => throw null;
}

It might not be a problem any longer for the stub generator, in which case the WithMetadataImportOptions might also give an alternative way of doing the filtering.

I vaguely remember that we started extracting private symbols because we needed them for stub generation. This is the PR where this was changed: #6000 and it also links to this #5664.

We needed these additional symbols when stubbing the .net framework, but I don't remember any longer for which API. The below might or might not be the case that caused the problem. I think when the below was extracted into a DB without private symbols, then we couldn't generate a stub that compiled.

public abstract class A
{
    public A(int i) => throw null;
}
public class B : A
{
    private B(int i) : base(i) => throw null; // not in the DB
    public B(int i, string s): this(i) => throw null;
}

It might not be a problem any longer for the stub generator, in which case the WithMetadataImportOptions might also give an alternative way of doing the filtering.

Thank you for the historical perspective! That makes sense.
The new stub generator that @hvitved made is no longer based on the extractor (or a DB produced by the extractor) - so that shouldn't be a problem.

One additional thing to consider: not extracting private fields can have an impact on MaD rows that use .Field[...] with a private field's name. I think these would need to be changed to .SyntheticField[...].

The QA run on 5k repos shows

Which indeed looks good (most likely the missing result for cs/threat-unsafe-icryptoptransform-field-in-class is due to the error in the query that @hvitved mentions above. I don't see any need to dig further into the results).

The following looks very good:

Faster extraction, faster trap import, faster query execution. Roughly 10% across the line and an 8% end-to-end performance improvement!

Link to QA dashboard: https://github.com/github/codeql-qa-ops/issues/240#issuecomment-2513329387

One additional thing to consider: not extracting private fields can have an impact on MaD rows that use .Field[...] with a private field's name. I think these would need to be changed to .SyntheticField[...].

That is a good point as well.
I tried to add the following code in the ModelValidation module in ExternalFlow.qll and execute this against a dotnet/runtime database (created from this branch) to check if there are any models pertaining to fields not extracted in dotnet/runtime.

  private predicate myTest(string field) {
    exists(AccessPath p, AccessPathToken token |
      token = p.getToken(_) and
      token.getName() = "Field" and
      field = token.getArgument(0) and
      not exists(Field f | f.getFullyQualifiedNameDebug() = field)
    )
  }

It appears that all the models we only use effectively public fields on dotnet/runtime: This makes sense as the only manual models we have that uses Field are for tuples (and these are public fields) and the model generator only creates models using Field in case a field is effectively public (otherwise it uses SyntheticField).